-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add data point for IndexedDB improved error reporting for large value read failures #25277
Add data point for IndexedDB improved error reporting for large value read failures #25277
Conversation
api/IDBRequest.json
Outdated
"improved_reporting_large_value_read_failures": { | ||
"__compat": { | ||
"description": "Improved reporting for large value read failures", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you point me to the part of the spec that mentions this, and/or the spec PR that added this behavior to the spec?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@caugner I'm pretty sure this is the one: w3c/IndexedDB#430.
The exceptions are listed in the spec at https://www.w3.org/TR/IndexedDB/#exceptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on w3c/IndexedDB#423, I would suggest to rename the feature as follows:
"improved_reporting_large_value_read_failures": { | |
"__compat": { | |
"description": "Improved reporting for large value read failures", | |
"throws_NotReadableError": { | |
"__compat": { | |
"description": "Throws `NotReadableError` on storage read failures", |
My impression is that "large value" is Chrome terminology, not part of the spec.
/cc @Elchi3 for a second opinion, as we may see more of these very specific behavioral features in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PS: We could add this spec_url: https://www.w3.org/TR/IndexedDB/#:~:text=NotReadableError
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like @caugner's description and ID a lot more but I also find it strange to use both, sub features and array support statements here. Is this the summary?
"chrome": [
{
"version_added": "132",
"notes": "Throws `NotReadableError` and `UnknownError`"
},
{
"version_added": "130",
"notes": "Throws `NotFoundError` and `DataError`"
},
{
"version_added": "48",
"notes": "Throws `DOMExceptions`"
},
{
"version_added": "23",
"notes": "Throws `DOMError`"
}
],
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From reading the content discussion, I think "Transient and unrecoverable read errors" (transient_unrecoverable_read_errors
) is a good feature name and id.
"Improved reporting for read errors" and then only setting Chrome to support and other browsers to false
seems wrong given this is about Chrome catching up and not providing "improvements" here, right? If the other browser support it, then we should say so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the feature name as suggested.
I agree it would be nice to include other browser support information, but I've got no idea how to find that out. I can't find a mention of NotReadableError
in the Gecko source code so, although they do support the same kind of error handling for read errors, I'm not sure if it is using the same exception names.
From the spec bug, I am wondering whether this has bene agreed as the error to put in the spec, which matches Fx behavior, but Fx hasn't actually implemented the official specified error yet. In which case, could we call it something like "Specified reporting for read errors", and it would be accurate to say that only Chrome has implemented it so far?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% sure, but most likely it was Firefox 14 that implemented NotReadableError
in https://bugzilla.mozilla.org/show_bug.cgi?id=730161, cf. mozilla/gecko-dev@1ce6cdf#diff-38a23a8920ed3a6e05068b536b2ba2f20b2ef6449a7a084ee0bc76faeca8e6d5R45
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Webkit most likely implemented it in https://bugs.webkit.org/show_bug.cgi?id=102137 / WebKit/WebKit@b368491, i.e. Safari 537.43.1 before Safari 7 (537.71).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds good enough for me ;-)
api/IDBRequest.json
Outdated
"improved_reporting_large_value_read_failures": { | ||
"__compat": { | ||
"description": "Improved reporting for large value read failures", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on w3c/IndexedDB#423, I would suggest to rename the feature as follows:
"improved_reporting_large_value_read_failures": { | |
"__compat": { | |
"description": "Improved reporting for large value read failures", | |
"throws_NotReadableError": { | |
"__compat": { | |
"description": "Throws `NotReadableError` on storage read failures", |
My impression is that "large value" is Chrome terminology, not part of the spec.
/cc @Elchi3 for a second opinion, as we may see more of these very specific behavioral features in the future.
api/IDBRequest.json
Outdated
{ | ||
"version_added": "130", | ||
"partial_implementation": true, | ||
"notes": "Throws a `NotFoundError` for unrecoverable large value read failures, and a `DataError` for transient large value read failures." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that the NotFoundError
has been in the spec since at least 2011 (before it was referred to as NOT_FOUND_ERR
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true. It is only mentioned here because Chrome reported that error type for read errors for a couple of versions (incorrectly, I assume), before it was then updated to NotReadableError
.
Co-authored-by: Claas Augner <[email protected]>
Co-authored-by: Claas Augner <[email protected]>
Co-authored-by: Claas Augner <[email protected]>
Co-authored-by: Florian Scholz <[email protected]>
…lures' of github.com:chrisdavidmills/browser-compat-data into indexeddb-improved-error-reporting-large-value-read-failures
@caugner @Elchi3 I had to make some further updates due to linting failures. I noticed that the new data point had been put at the wrong nesting level (oops!), and I had to adjust the support versions for FxA and Saf because the specified values were earlier than the support values for the parent feature. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good and plausible to me.
Summary
Chrome has updated its error reporting for large value read failures with transient causes such as low memory and unrecoverable causes such as source blob files being deleted.
Chrome 130 updates the
DOMException
types to be more appropriate, and Chrome 132 further updates them and improves the associated error messages.See https://chromestatus.com/feature/5140210640486400 for the data source.
Test results and supporting details
Related issues